Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec for Elevation QOL improvements #8455

Merged
merged 16 commits into from
Aug 25, 2021
Merged

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Dec 1, 2020

doc link

Summary of the Pull Request

Despite my best efforts to mix elevation levels in a single Terminal window, it seems that there's no way to do that safely. With the dream of mixed elevation dead, this spec outlines a number of quality-of-life improvements we can make to the Terminal today. These should make using the terminal in elevated scenarios better, since we can't have M/E.

Abstract

For a long time, we've been researching adding support to the Windows Terminal
for running both unelevated and elevated (admin) tabs side-by-side, in the same
window. However, after much research, we've determined that there isn't a safe
way to do this without opening the Terminal up as a potential
escalation-of-privilege vector.

Instead, we'll be adding a number of features to the Terminal to improve the
user experience of working in elevated scenarios. These improvements include:

PR Checklist

Detailed Description of the Pull Request / Additional comments

*** read the spec ***

Why are these two separate documents?

I felt that the spec that is currently in review in #7240 and this doc should remain separate, yet closely related documents. #7240 is more about showing how this large set of problems discussed in #5000 can all be solved technically, and how those solutions can be used together. It establishes that none of the proposed solutions for components of #5000 will preclude the possibility of other components being solved. What it does not do however is drill too deeply on the user experience that will be built on top of those architectural changes.

This doc on the other hand focuses more closely on a pair of scenarios, and establishes how those scenarios will work technically, and how they'll be exposed to the user.

TODO:

  • Discuss how a cross-elevation splitPane should work
  • Add a warning dialog before auto-elevating a profile. Otherwise someone might inject malicious.exe into your auto-elevate profile, and have you run it without knowing it was changed.
  • Get someone from the security team's eyes on this
  • There's Allow for custom profiles when running as admin vs user #3637 tracking "different profiles if we're elevated or unelevated". I'm not sure if the current solution will work for this.
    • I should add a section on "hiddenWhenElevated"/"hiddenWhenUnelevated" to future considerations for this

@zadjii-msft zadjii-msft added Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. labels Dec 1, 2020
@skyline75489
Copy link
Collaborator

skyline75489 commented Dec 3, 2020

Should the roadmap be updated accordingly? I mean in the “open tab as admin” section in
https://github.com/microsoft/terminal/blob/main/doc/terminal-v2-roadmap.md#20-scenarios

@zadjii-msft
Copy link
Member Author

Should the roadmap be updated accordingly?

meh, probably. Could use some editorializing to reflect the current state of the #5000 plans. Thanks for bringing that up!

@zadjii-msft
Copy link
Member Author

Note to reviewers: I'm probably going to be changing this from elevated: bool? to elevate: bool, where false does nothing and true will elevate if it's not already elevated. TURNS OUT that centenial doesn't play super well with de-elevating a process.

comfortable shipping a mixed-elevation solution, because there's simply no way
for us to be confident that we haven't introduced an escalation-of-privilege
vector utilizing the Terminal. No matter how small the attack surface might be,
we wouldn't be confident that there are _no_ vectors for an attack.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially given our roadmap has us replacing the default story with the Terminal. And we don't want to be vulnerable by default. Individually choosing to be vulnerable... that's different.

and have the elevated window open with the profile automatically.

We'll be adding the `"elevate": true|false` setting as a per-profile setting,
with a default value of `false`. When set to `true`, we'll try to auto-elevate
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe display the actual command before auto-elevate the profile? Malicious software could possibly modify the elevated profiles and perform some kind of injection. Users will not notice the difference if the profile is auto-elevated, and naturally say "yes" to some malwares unconsciously.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe UAC will do that already, though, it's in the "show details". I suppose we'll only be showing the profile... I guess that means someone could edit the settings so the profile was "commandline": "malicious.exe", and there's nothing preventing them from doing that. That's not good. Yea there should probably be another internal warning.

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm good with this. Thanks for the additional evaluation of the previous batch of comments, especially the future considerations once we get a bit further so we can refer back.

@zadjii-msft zadjii-msft added this to the Terminal v1.7 milestone Feb 2, 2021
@github-actions

This comment has been minimized.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Aug 17, 2021
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanna iron out a few more thoughts.

Did we ever have a spec meeting on this? Do you want to have one? It's been a while haha

Comment on lines +129 to +131
One easy way of doing this is by adding a simple UAC shield to the left of the
tabs for elevated windows. This shield could be configured by the theme (see
[#3327]). We could provide the following states:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By making it a part of themes, that means it'll act like a global setting right? We won't have a scenario where you can have a different icon on each tab?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. This icon's only in the top-left of the entire window, not the tab.

One easy way of doing this is by adding a simple UAC shield to the left of the
tabs for elevated windows. This shield could be configured by the theme (see
[#3327]). We could provide the following states:
* Colored (the default)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be cool if you could upload your own icon too (similar to profile icons). That should be pretty straightforward.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, yea, sure, we could do that if someone really wanted that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd swing hard the other way -- why let them choose mono or color? Just "on or off", with us controlling the visual treatment. for now

For the following examples, let's assume the user is currently in an unelevated
Terminal window.

When the user tries to create a new elevated **tab**, we'll need to create a new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could/Should we add something like wt --admin new-tab ...?

Note --admin is off of wt not new-tab. It would be annoying to have to set the elevate member on NewTerminalArgs for each command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might also solve the split-pane issue below? I feel like it's more straightforward to thing of the entire wt.exe commandline as "do all of this in an admin window" vs "do some of these commands in an admin window"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's maybe a good tangent to the split-pane discussion. With that discussion, I'm more concerned about someone trying to splitPane with a keybinding in an unelevated window, a Profile that's elevate=true. That's where it's a little weird.

wt --admin [args...] would be interpreted as basically "if I'm not elevated, launch an elevated wt [args...]". wt -w X --admin [args...] would then run [args...] in the elevated X window

Comment on lines +503 to +506
* Over the course of discussion concerning appearance objects ([#8345]), it
became clear that having separate "elevated" appearances defined for
`profile`s was overly complicated. This is left as a consideration for a
possible future extension that could handle this scenario in a cleaner way.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have an issue to track the discussion on "elevated" vs "unfocused" appearance configs?

I'm wondering if we could just do the following:

Focused Unfocused
Regular regular settings unfocusedAppearance
Elevated elevatedAppearance elevatedAppearance

Reasoning: "elevated" is a long-term state whereas focus constantly shifts

idk, either way, a thread on this might be nice to have.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, we might as well just use #1939 for that, yea?

Comment on lines +507 to +510
* Similarly, we're going to leave [#3637] "different profiles when elevated vs
unelevated" for the future. This also plays into the design of "configure the
new tab dropdown" ([#1571]), and reconciling those two designs is out-of-scope
for this particular release.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crazy idea: admin-settings.json?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I'm just thinking newTabDropdown and adminNewTabDropdown, if you really want them to be different

Copy link
Contributor

@Rosefield Rosefield Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think that having completely separate settings would prevent security issues in the future. It may not be the best UX, but the safest thing would probably be to only have a OpenNewAdminWindow that just does runas wt.exe, and then within the admin window they can interact with the terminal however they'd like with the admin settings. I think that marshaling any amount of data into an admin context could cause problems. This is assuming that the actual wt.exe is only modifiable by the administrator.

consider if the attacker tries to run something like (edit: On reflection, the fact that you can just run a command as administrator is something that probably can't be prevented)

wt.exe "cmd.exe {1000 spaces/newlines to hide the rest of the string} && evil.exe"

Separate settings does have benefits too

  • admin specific startup actions
  • admin only profiles for free (including styling)

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 18, 2021
Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this. Thanks!

@zadjii-msft
Copy link
Member Author

Did we ever have a spec meeting on this? Do you want to have one? It's been a while haha

Lol yea, I'm pretty sure we actually did have one one this. IIRC it was during the other process model one? In like, March 😝 The new stuff we discussed Monday, so I figured I'd document and #shipit

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix these and then GO OG OG

@github-actions
Copy link

github-actions bot commented Aug 25, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • incrememntally
Previously acknowledged words that are now absent SPACEBAR Unregister xIcon yIcon
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the dev/migrie/s/1032-elevation-qol branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/1b6e6bd6dd03b72e2918a076eea8f701a71455ab.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/905733004" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@zadjii-msft zadjii-msft merged commit f7b0f74 into main Aug 25, 2021
@zadjii-msft zadjii-msft deleted the dev/migrie/s/1032-elevation-qol branch August 25, 2021 17:42
ghost pushed a commit that referenced this pull request Sep 23, 2021
## Summary of the Pull Request

Adds a visible indicator that a Terminal window is elevated. This icon can be disabled with `"showAdminShield" false` in the global settings.

## References

* spec'd in #8455 
* Also in https://github.com/microsoft/terminal/projects/5
* big picture: #5000

## PR Checklist
* [x] Closes #1939
* [x] I work here
* [n/a] Tests added/passed
* [ ] Requires documentation to be updated - yea probably

## Validation Steps Performed

![image](https://user-images.githubusercontent.com/18356694/133293009-4215e319-fbf9-4ca8-8af5-afe2fa8bb62d.png)

![image](https://user-images.githubusercontent.com/18356694/133292970-90cb17fd-16c7-429a-a25f-8457850eb278.png)
ghost pushed a commit that referenced this pull request Jan 12, 2022
## Summary of the Pull Request

This is the resurrection of #8514 and #11310. WE determined that we didn't want to do #11308 after all, so this should be profile auto-elevation, without the warning.

This PR adds two features:
* the `elevate: bool` property to profiles
  - If the user is running unelevated, **and** `elevate` is set to `true`, then instead of opening a new tab, we'll open an elevated Terminal window with the profile.
  - Otherwise, we'll just open a new tab in the existing window. This includes cases where the window is elevated, and the profile is set to `elevate:false`. `elevate:false` basically just means "do nothing special with me".
* the `elevate: bool?` property to `NewTerminalArgs` (`newTab`, `splitPane`)
  - This allows a user to create an action that will elevate the profile, even if the profile is not otherwise set to auto-elevate.
  - `elevate:null` (_the default_) does not change the profile's elevation status. The action will use whatever is set by the profile.
  - `elevate:true` will attempt to auto-elevate the profile
  - `elevate:false` will do nothing special. 


## References
* #5000 for obvious reasons
* Spec'd in #8455

## PR Checklist
* [x] Closes #632
* [x] I work here
* [x] Tests added/passed
* [ ] Requires documentation to be updated - sure does, but that'll come all at the end.

## Detailed Description of the Pull Request / Additional comments

After playing with de-elevation a bit, it turns out it behaves weirdly with packaged applications. I can try and ask `explorer.exe` to launch the process on our behalf. However, if the thing we're launching is an execution alias (`wt.exe`), and we're elevated, then the child process will _still launch elevated_. 

There's also something super BODGEY at work here. `ShellExecute` is the function we use to ask the OS to elevate something for us. But `ShellExecute` needs to be able to send a window message to the process that called it (if the caller was a WINDOWS subsystem application). So if we die immediately after calling `ShellExecute`, then the elevated process never actually spawns - sad. So we're adding a helper process, `elevate-shim.exe`, that lives in our process. That'll be the one that actually calls `ShellExecute`, so that it can live for the duration of the UAC prompt.

## Validation Steps Performed

* Ran tests
* Opened a bunch of terminal tabs at various different elevation levels
* opened new splits too
* In the defaults (base layer) as well, for madness 

Some settings to use for testing

<details>

```jsonc
    "keybindings" :
    [
        ////////// ELEVATION ///////////////
        { "keys": "f1", "name": "ELEVATED TAB", "icon": "\uEA18", "command": { "action": "newTab", "elevate": true } },
        { "keys": "f2", "name": "ELEVATED, Color", "icon": "\uEA18", "command": {
            "action": "newTab", "elevate": true, "commandline": "PowerShell.exe", "startingDirectory": "C:\\Windows", "tabColor": "#bbaa00"
        } },
        { "keys": "f3", "name": "unelevated ELEVATED", "icon": "🙃", "command": {
            "action": "newTab", "elevate": false, "profile": "elevated cmd"
        } },
        //////////////////////////////
    ],

    "profiles":
    {
        "defaults":
        {
            "elevate": true,
        },
        "list":
        [
            {
                "hidden":false,
                "name" : "cmd",
                "commandline" : "cmd.exe",
                "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
                "startingDirectory" : "%USERPROFILE%",
                "opacity" : 20
            },
            {
                "name" : "the COOLER cmd",
                "commandline" : "c:\\windows\\system32\\cmd.exe",
                "startingDirectory" : "%USERPROFILE%",
            },
            {
                "name" : "the sneaky cmd",
                "commandline" : "c:\\windows\\system32\\cmd.exe /k echo sneaky sneaks",
                "startingDirectory" : "%USERPROFILE%",
            },
            {
                "name": "elevated cmd",
                "commandline": "cmd.exe /k echo This profile is always elevated",
                "startingDirectory" : "well this is garbage",

                "elevate": true,
                "background": "#9C1C0C",
                "tabColor": "#9C1C0C",
                "colorScheme": "Desert"
            },
            {
                "name": "unelevated cmd",
                "commandline": "cmd.exe /k echo This profile is just as elevated as you started with",
                "elevate": false,
                "background": "#1C0C9C",
                "tabColor": "#1C0C9C",
                "colorScheme": "DotGov",
                "useAcrylic": true
            },
        ]
```

</details>

Also try:
* `wtd nt -p "elevated cmd" ; sp -p "elevated cmd"`
* `wtd nt -p "elevated cmd" ; nt -p "elevated cmd"`




This was merged manually via 

```
git diff dev/migrie/f/non-terminal-content-elevation-warning dev/migrie/f/632-on-warning-dialog > ..\632.patch
git apply ..\632.patch --ignore-whitespace --reject
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants